Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow callables for 'default' in schema (and minor fixes) #200

Merged
merged 4 commits into from
Feb 8, 2016

Conversation

dkellner
Copy link
Contributor

@dkellner dkellner commented Feb 3, 2016

Allows the use of computed default values via callables / lambda functions. For an example why this is useful, see https://github.com/nicolaiarocci/eve/pull/815.

This implementation is slightly different (and simpler) than the one in the pull request mentioned above, as here only the current (sub)document is passed to the callable. Passing the whole document seemed unnecessarily complex as in that case we would have to take care of the evaluation order of child validators, too. The downside is, of course, that you can only access fields in your current "nesting level".

@funkyfuture
Copy link
Member

The downside is, of course, that you can only access fields in your current "nesting level".

there's no need for child-validators to achieve that. to access the whole context it requires the possibility to use methods as callables.
that would imply that the default rule for callables should be named default_handler (similar to rename_handler) or alike to discriminate string values from methods.

i will probably do a code review this evening in europe.

@funkyfuture
Copy link
Member

here are my thoughts:

test_depending_... and test_default_callable_with_multiple_dependencies would be better readable if the keys in the schema were sorted alphabetically .

can circular dependencies be detected while schema validation? if yes, that would be preferable since it's easier to deal with errors prior to validation in client code and the document processing itself would be more lightweight.

@nicolaiarocci
Copy link
Member

I love this. Detecting circular dependencies during schema validation would be great.

@dkellner
Copy link
Contributor Author

dkellner commented Feb 5, 2016

test_depending_... and test_default_callable_with_multiple_dependencies would be better readable if the keys in the schema were sorted alphabetically .

You're right, I'm going to change this. The initial thought was to enforce a situation where _normalize_default has to delay the processing of at least one callable. But as the order of dict keys isn't predictable it does not make much sense to write it that way.

there's no need for child-validators to achieve that. to access the whole context it requires the possibility to use methods as callables.
that would imply that the default rule for callables should be named default_handler (similar to rename_handler) or alike to discriminate string values from methods.

Thanks for pointing to the rename_handler. I will delve deeper into the codebase over the weekend and try to make more sense of your comment after that ;).

Checking for circular dependencies during schema validation sounds good, but I'm afraid that will cover only "simple" cases (but those are probably the most common ones). I'd leave the error handling in _normalize_default, too. Otherwise we could end up in an infinite loop for more complex callables, e.g. callables that behave differently based on other document fields.

@funkyfuture
Copy link
Member

another thought:

why check for circular references at all? if such occurs, it's completely the developer's responsibility, right? i'm very much for anticipating user's mistakes, but i think developers can be left with their own fate when it becomes too complex to protect them from themselves. and there are certainly more potential mistakes a developer could make than circular references. so, essentially: every developer should test her/his code well.

@funkyfuture
Copy link
Member

funkyfuture commented Feb 5, 2016 via email

@nicolaiarocci nicolaiarocci merged commit 620c10e into pyeve:master Feb 8, 2016
nicolaiarocci added a commit that referenced this pull request Feb 8, 2016
@nicolaiarocci
Copy link
Member

Awesome. Thank you.

@dkellner
Copy link
Contributor Author

dkellner commented Feb 8, 2016

You're fast - this wasn't meant to be merged right away :). This is what the (wip) = (work in progress) should indicate. Sorry for the confusion!

I'd like to remove the commented out test at least and maybe add some comments for clarification, shall I open another Pull Request?

@nicolaiarocci
Copy link
Member

ah sorry about that. yup open another pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants